GH-50567: [C++] Introduce JsonWriter and migrate integration JSON writer#50568
GH-50567: [C++] Introduce JsonWriter and migrate integration JSON writer#50568Reranko05 wants to merge 9 commits into
Conversation
|
|
fd69d50 to
bb7fd90
Compare
|
@kou could you review this PR when you have a chance? |
kou
left a comment
There was a problem hiding this comment.
Could you check performance difference?
|
@kou, I compared the serialization performance of main: 2157.4 ms I didn't observe any measurable performance regression. |
|
@kou I investigated the failing workflows and they appear to be unrelated to this PR. Could you please verify? |
bb7fd90 to
c983eb0
Compare
There was a problem hiding this comment.
Thanks for working on this @Reranko05! Exciting to see this work!
We should probably add meson support for json_writer, see (cpp/src/arrow/meson.build, cpp/src/arrow/json/meson.build, cpp/src/arrow/integration/meson.build).
|
Another thought - do we have conbench cpp micro benchmarks to monitor improvement of json parsing and writing? |
There was a problem hiding this comment.
Pull request overview
This PR adds a reusable arrow::json::JsonWriter wrapper around simdjson’s builder API and migrates the C++ integration JSON writer (and its tests) away from RapidJSON’s writer, as part of the incremental RapidJSON → simdjson migration.
Changes:
- Added
arrow::json::JsonWriter(header/impl) plus unit tests. - Migrated integration JSON serialization code paths to emit JSON using
JsonWriter. - Updated CMake/test linkage and CI environment to ensure simdjson is available/linked where needed.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/src/arrow/json/json_writer.h | Introduces the JsonWriter public API. |
| cpp/src/arrow/json/json_writer.cc | Implements JsonWriter on top of simdjson builder primitives. |
| cpp/src/arrow/json/json_writer_test.cc | Adds unit coverage for basic writer operations. |
| cpp/src/arrow/json/CMakeLists.txt | Adds the new unit test and links simdjson for the test target. |
| cpp/src/arrow/integration/json_internal.h | Updates writer function signatures to accept arrow::json::JsonWriter*. |
| cpp/src/arrow/integration/json_internal.cc | Migrates integration JSON emission to JsonWriter (writer-side). |
| cpp/src/arrow/integration/json_integration.cc | Switches IntegrationJsonWriter implementation to JsonWriter. |
| cpp/src/arrow/integration/json_integration_test.cc | Updates integration tests to use JsonWriter for schema/array JSON generation. |
| cpp/src/arrow/integration/CMakeLists.txt | Links simdjson where integration tests/executables now depend on it. |
| cpp/src/arrow/CMakeLists.txt | Adds json/json_writer.cc to Arrow JSON sources and links simdjson for integration targets. |
| ci/docker/ubuntu-24.04-cpp.dockerfile | Sets simdjson_SOURCE=BUNDLED to ensure simdjson availability in CI images. |
| ArrayWriter(const std::string& name, const Array& array, JsonWriter* writer) | ||
| : name_(name), array_(array), writer_(writer) {} |
I wasn't able to find any existing Conbench C++ microbenchmarks covering JSON parsing or writing. I did run a local benchmark comparing |
cfe491e to
a25a245
Compare
ff3b25e to
c461c63
Compare
|
Marking this as ready for review. The remaining CI failures appear to be unrelated to this change (dependency download, environment, or infrastructure issues). |
|
Hi @kou! Would it be possible to assign the issue to me? I think that would help avoid duplicate work while the RapidJSON → simdjson migration is in progress. Thanks! |
|
It seems that the issue is already assigned to you: #50567 (comment) |
|
Could you share the naive |
|
@kou Sure
#pragma once
#include <cstdint>
#include <sstream>
#include <string>
#include <string_view>
#include "arrow/util/visibility.h"
namespace arrow::json {
class ARROW_EXPORT JsonWriter {
public:
JsonWriter() = default;
void StartObject();
void EndObject();
void StartArray();
void EndArray();
void Key(std::string_view key);
void String(std::string_view value);
void RawValue(std::string_view value);
void Bool(bool value);
void Int(int32_t value);
void Int64(int64_t value);
void Uint(uint32_t value);
void Uint64(uint64_t value);
void Double(double value);
void Null();
std::string_view GetString() const;
void Clear();
private:
void MaybeComma();
void WriteQuotedString(std::string_view value);
std::ostringstream output_;
mutable std::string output_cache_;
bool needs_comma_ = false;
};
} // namespace arrow::json
#include "arrow/json/json_writer_internal.h"
namespace arrow::json {
void JsonWriter::StartObject() {
MaybeComma();
output_ << '{';
needs_comma_ = false;
}
void JsonWriter::EndObject() {
output_ << '}';
needs_comma_ = true;
}
void JsonWriter::StartArray() {
MaybeComma();
output_ << '[';
needs_comma_ = false;
}
void JsonWriter::EndArray() {
output_ << ']';
needs_comma_ = true;
}
void JsonWriter::Key(std::string_view key) {
MaybeComma();
WriteQuotedString(key);
output_ << ':';
needs_comma_ = false;
}
void JsonWriter::String(std::string_view value) {
MaybeComma();
WriteQuotedString(value);
needs_comma_ = true;
}
void JsonWriter::RawValue(std::string_view value) {
MaybeComma();
output_ << value;
needs_comma_ = true;
}
void JsonWriter::Bool(bool value) {
MaybeComma();
output_ << (value ? "true" : "false");
needs_comma_ = true;
}
void JsonWriter::Int(int32_t value) {
MaybeComma();
output_ << value;
needs_comma_ = true;
}
void JsonWriter::Int64(int64_t value) {
MaybeComma();
output_ << value;
needs_comma_ = true;
}
void JsonWriter::Uint(uint32_t value) {
MaybeComma();
output_ << value;
needs_comma_ = true;
}
void JsonWriter::Uint64(uint64_t value) {
MaybeComma();
output_ << value;
needs_comma_ = true;
}
void JsonWriter::Double(double value) {
MaybeComma();
output_ << value;
needs_comma_ = true;
}
void JsonWriter::Null() {
MaybeComma();
output_ << "null";
needs_comma_ = true;
}
std::string_view JsonWriter::GetString() const {
output_cache_ = output_.str();
return output_cache_;
}
void JsonWriter::Clear() {
output_.str("");
output_.clear();
output_cache_.clear();
needs_comma_ = false;
}
void JsonWriter::MaybeComma() {
if (needs_comma_) {
output_ << ',';
}
}
void JsonWriter::WriteQuotedString(std::string_view value) {
output_ << '"';
for (char c : value) {
switch (c) {
case '"':
output_ << "\\\"";
break;
case '\\':
output_ << "\\\\";
break;
case '\b':
output_ << "\\b";
break;
case '\f':
output_ << "\\f";
break;
case '\n':
output_ << "\\n";
break;
case '\r':
output_ << "\\r";
break;
case '\t':
output_ << "\\t";
break;
default:
output_ << c;
break;
}
}
output_ << '"';
}
} // namespace arrow::jsonAdd temporary benchmark in #include <chrono>
#include <iostream>TEST(JsonWriter, BenchmarkSerialization) {
JsonWriter writer;
constexpr int kIterations = 1000000;
auto start = std::chrono::steady_clock::now();
for (int i = 0; i < kIterations; ++i) {
writer.Clear();
writer.StartObject();
writer.Key("id");
writer.Int(i);
writer.Key("name");
writer.String("Apache Arrow");
writer.Key("value");
writer.Double(3.1415926535);
writer.Key("valid");
writer.Bool(true);
writer.EndObject();
auto s = writer.GetString();
(void)s;
}
auto end = std::chrono::steady_clock::now();
auto ms =
std::chrono::duration_cast<std::chrono::milliseconds>(end - start)
.count();
std::cout << "Time: " << ms << " ms\n";
} |
What changes are included?
This PR introduces a reusable
JsonWriterwrapper around simdjson's JSON builder API and migrates the integration JSON writer to use it instead of RapidJSON.Specifically, this PR:
JsonWriterabstraction inarrow/json.JsonWriter.JsonWriter.JsonWriter.This is part of the incremental migration from RapidJSON to simdjson.
Performance
I compared the serialization performance of
IntegrationJsonWriter::WriteRecordBatch()+Finish()using a temporary benchmark with a 1M-rowRecordBatch. The benchmark was run 10 times on bothmainand this branch.mainNo measurable performance regression was observed.
Are these changes tested?
Yes.
I added unit tests for
JsonWriterand verified that both JSON and integration tests pass locally:arrow-json-testarrow-json-integration-testAre there any user-facing changes?
No.